fix: filter secrets context from ignore-if-missing if: guard expression#45828
Conversation
… expressions
GitHub Actions only allows github, needs, vars, env, inputs, steps, and runner
contexts in if: expressions. The secrets context is not allowed.
When buildIgnoreIfMissingCondition generates the step guard expression for
ignore-if-missing: true, it now filters out any wrapped ${{ secrets.* }}
values. Only expressions with valid if: contexts (vars, env, inputs, github,
steps, runner, etc.) are included in the condition. When all credential values
use the secrets context, no if: condition is emitted and the step runs
unconditionally (same as the workaround users had found).
Fixes: #44709
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…ontextPrefixes Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #45828 does not have the 'implementation' label and has 81 new lines of code in business logic directories (below the 100-line threshold). |
|
✅ Test Quality Sentinel completed test quality analysis. |
There was a problem hiding this comment.
Pull request overview
Updates GitHub App token guards to avoid unsupported secrets.* references in step conditions.
Changes:
- Filters selected contexts from generated credential guards.
- Omits empty guards across four token-mint paths.
- Updates tests for secret-backed credentials.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/safe_outputs_app_config.go |
Implements filtering and conditional guard emission. |
pkg/workflow/compiler_pre_activation_job.go |
Handles empty pre-activation guards. |
pkg/workflow/compiler_activation_daily_aic.go |
Preserves the daily AIC condition when credential guards are empty. |
pkg/workflow/safe_outputs_app_test.go |
Updates safe-output guard tests. |
pkg/workflow/skip_if_match_test.go |
Updates pre-activation guard expectations. |
pkg/workflow/activation_github_token_test.go |
Updates activation guard expectations. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 6/6 changed files
- Comments generated: 3
- Review effort level: Medium
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Two correctness issues + one consistency gap need resolution before merge
The core direction of this PR is correct — emitting secrets.* in if: expressions is a GitHub Actions parse error. But the implementation has gaps:
isValidIfContextExpressiononly checks the leading prefix (see inline comment on line 67). A composite expression where an invalid context appears mid-string will silently pass and produce a brokenif:.- Empty-string return = unconditional execution, which directly defeats
ignore-if-missingsemantics when both credentials are secrets-based. The step will now fail at runtime instead of being skipped, which is worse than the bug being fixed (see inline comment on line 97). - Inconsistent fallback behaviour across the four callers:
buildDailyAICAppTokenMintStephas anelsebranch, butbuildPreActivationAppTokenMintStepand the two insafe_outputs_app_config.godo not. Needs explicit documentation or alignment (see inline comment on compiler_pre_activation_job.go:43).
🔎 Code quality review by PR Code Quality Reviewer · 33.6 AIC · ⌖ 4.81 AIC · ⊞ 5.6K
Comment /review to run again
Comments that could not be inline-anchored
pkg/workflow/safe_outputs_app_config.go:67
isValidIfContextExpression only checks the leading prefix — a composite expression like ${{ github.actor != '' || secrets.TOKEN != '' }} passes this check (leading token is github.) but GitHub Actions will still reject the if: at parse time because secrets. appears in the expression body.
<details>
<summary>💡 Suggested fix</summary>
Use strings.Contains instead of strings.HasPrefix to catch invalid context references anywhere in the expression:
func isValidIfContextE…
</details>
<details><summary>pkg/workflow/safe_outputs_app_config.go:97</summary>
**Returning `""` when all credentials are invalid-context breaks `ignore-if-missing` semantics** — when both `AppID` and `PrivateKey` reference `secrets.*`, every check is filtered out and the function returns `""`. All callers then omit the `if:` guard entirely, making the token-mint step run unconditionally. If the secrets are not configured, the step will **fail at runtime** rather than skip. The whole point of `ignore-if-missing: true` is to skip when credentials are absent; silently runnin…
</details>
<details><summary>pkg/workflow/compiler_pre_activation_job.go:43</summary>
**No fallback `if:` guard when condition is empty, inconsistent with `compiler_activation_daily_aic.go`** — in `buildDailyAICAppTokenMintStep`, an empty condition correctly falls back to `maxDailyAICreditsConfiguredIfExpr`. Here in `buildPreActivationAppTokenMintStep`, an empty condition produces no `if:` at all (step always runs), with no equivalent fallback. If these two code paths are intentionally different, that should be documented; if not, this is a behavioural inconsistency.
<details>
…
</details>
There was a problem hiding this comment.
The fix is correct and complete.
What was changed and why it works:
buildIgnoreIfMissingConditionnow filters outsecrets.*,jobs.*, andmatrix.*context expressions before building the&&-joined guard, matching the GitHub Actions rule that onlygithub,needs,vars,env,inputs,steps, andrunnerare valid inif:conditions.- Returns an empty string when all credentials use invalid contexts, and all 4 callers were updated correctly (step runs unconditionally — the safe fallback).
- Tests cover all three meaningful cases: both secrets (empty guard), mixed vars+secrets (vars-only guard), both vars (full guard).
Minor non-blocking note: In compiler_activation_daily_aic.go, the inner else and outer else both emit identical if: maxDailyAICreditsConfiguredIfExpr lines — slight duplication but not a bug.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 13.2 AIC · ⌖ 5.2 AIC · ⊞ 5K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — the fix is correct and well-targeted; two minor suggestions filed as inline comments.
📋 Key Themes & Highlights
Key Themes
jobs.andmatrix.coverage gap — both prefixes are inifInvalidContextPrefixesbut have no test cases inTestBuildIgnoreIfMissingCondition.- Silent guard omission — when all credentials use
secrets.*, theif:guard is silently dropped. The per-credential log line fires, but nothing signals at compile time that the whole guard was omitted.
Positive Highlights
- ✅ Blocklist + helper pattern makes the invariant explicit and easy to extend.
- ✅ Empty-string sentinel for "no guard" is adopted consistently across all four callers.
- ✅ New test table covers the most important mixed-context scenario (
vars+secrets). - ✅ PR description table cleanly documents the three behavioral cases.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 33.5 AIC · ⌖ 4.58 AIC · ⊞ 6.7K
Comment /matt to run again
🧪 Test Quality Sentinel Report✅ Test Quality Score: 81/100 — Acceptable
📊 Metrics (6 tests)
📋 Test Changes Summary3 test files modified, 0 new tests, 6 tests updated:
✨ Quality Highlights
Verdict
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
There was a problem hiding this comment.
✅ Test Quality Sentinel: 81/100. All tests are design-focused (100%) with excellent multi-point integration coverage across safe_outputs, activation, and pre-activation jobs. The table-driven expansion adds genuine behavioral coverage for context filtering edge cases.
|
@copilot please run the
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
@copilot please run the
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Updated in 7a5bc2a. The branch now preserves |
…-missing-expression Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Updated on current HEAD |
|
@copilot please run the
|
🤖 PR Triage
Score breakdown: Impact 35 + Urgency 22 + Quality 13 Fixes compiler bug: Run §29462479491
|
Checked current HEAD |
|
@copilot please run the
|
Checked current HEAD |
|
\n@copilot please run the skill, address the unresolved review feedback (newest first: github-actions: #45828 (comment); github-actions: #45828 (comment); copilot-pull-request-reviewer: #45828 (comment); copilot-pull-request-reviewer: #45828 (comment); copilot-pull-request-reviewer: #45828 (comment)), and rerun checks once the branch is up to date.\n\n> Generated by \ud83c\udf73 PR Sous Chef
|
|
🎉 This pull request is included in a new release. Release: |
GitHub Actions rejects
secrets.*inif:expressions at parse time — onlygithub,needs,vars,env,inputs,steps, andrunnercontexts are valid there. Whenignore-if-missing: truewas set on a GitHub App config that used${{ secrets.* }}for either credential, the compiler emitted an invalid guard like:Changes
buildIgnoreIfMissingCondition— rewrote to filter out any${{ secrets.* }},${{ jobs.* }}, or${{ matrix.* }}expressions before building the&&-joined guard. Returns an empty string when all credentials use invalid contexts (step runs unconditionally, which is the correct fallback).ifInvalidContextPrefixes+isValidIfContextExpression— new package-level blocklist and helper encoding which expression context prefixes are not allowed inif:conditions, with a reference to the GitHub Actions docs.buildGitHubAppTokenMintStepWithMeta,buildActivationAppTokenMintStep,buildPreActivationAppTokenMintStep, andbuildDailyAICAppTokenMintStepnow guard against the empty-string return before emitting anif:line.Resulting behavior
client-idprivate-keyif:${{ vars.APP_ID }}${{ secrets.KEY }}${{ vars.APP_ID != '' }}${{ vars.APP_ID }}${{ vars.KEY }}${{ vars.APP_ID != '' && vars.KEY != '' }}${{ secrets.ID }}${{ secrets.KEY }}